fix(code-review): key signal cache on the diff, not the worktree - #192
fix(code-review): key signal cache on the diff, not the worktree#192mikeangstadt wants to merge 3 commits into
Conversation
- ISS-8961: the signals/ namespace key was (diff_tip, taxonomy_hash, prompt_hash), and diff_tip is a ref name -- literally "HEAD" for a local branch review -- so no component varied with the diff. The cache dir is ~/.claude/cr-cache-global-repo-<worktree basename>, so two reviews from one pooled directory shared directory and key, and the second was served the first's extracted signals. - Add signal_input_hash: SHA-256 over the canonical serialization of the agent input bundle extract-signals-prepare builds, which is exactly what the extraction agent reads. Key is now (diff_tip, input_hash, taxonomy_hash, prompt_hash). - Build the bundle before computing the key rather than only on a miss. - A bundle with no changed files has no diff identity: emit an empty cache_key plus cache_bypass_reason "no-changed-files", which suppresses the read and (via consolidate's empty-key guard) the write. - Correct the --diff-tip help, which claimed the flag was a SHA. Testing: pytest plugins/ (3 pre-existing failures unrelated to this change: TestOverrideCache/TestPR114ReviewFixes fail on clean main because their override fixture is dated 2026-05-29 and the overrides TTL is 90 days). Counterfactual: reverting code_review_helpers.py turns TestPooledWorktreeSignalCacheIsolation red with lane 2's manifest reading status "cache_hit". ruff check; pyright. Risks: cached extractions written under the old key scheme are misses after this lands, so the first review in each cache directory re-runs one Haiku extraction.
39acc8d to
880fad6
Compare
- Close a false green in the pooled-worktree regression test: it varied the changed PATH as well as the content, so a key hashing only the file list still passed. Add a lane pair holding path, status, and line counts identical and varying only the excerpt -- the shape two reviews of one branch actually take as it is amended. - Delegate signal_input_hash to _stable_json_hash, the module's existing owner of the deterministic cache-key JSON rule, instead of a byte-identical second copy of it. - Pin the bundle's run-to-run stability: a run-varying field added to _build_signal_input would make every review a permanent miss, and the golden harness cannot catch it because it normalizes cache_key. - Emit input_hash in the manifest beside taxonomy_hash and prompt_hash, so the one component that actually varies is visible when diagnosing a key move. Normalized in the golden harness like its siblings. - State the real reason the no-changed-files bundle bypasses the cache. It is not that such a bundle cannot be keyed -- it is the shape a degraded parse-diff takes, and caching it would persist a degraded extraction for the namespace TTL. Name the cost too: a genuinely empty review now re-dispatches its extraction every run. Testing: uv run pytest plugins/ -- 2113 passed, 3 pre-existing failures (override-TTL fixture dated 2026-05-29 vs a 90-day TTL, red on main). ruff check; uv run pyright 0 errors. Counterfactual: restoring origin/main's helper turns 13 tests red, including both pooled-lane tests. Mutation: narrowing the key to the file list alone is now killed by test_same_file_different_content_is_not_served_a_stale_hit. Risks: manifest gains an additive input_hash field; no consumer reads it, and the golden harness normalizes it.
|
Follow-up commit Closed a false green in my own regression test. SSOT. Run-to-run stability is now pinned. The highest-consequence regression this design admits is someone adding a run-varying field to Diagnosability. The manifest emitted Corrected the bypass rationale. The docstring claimed a zero-file bundle "has no diff identity to key on". Not true — it hashes fine, and two genuinely empty reviews would legitimately share an entry. The real reason is that it is the shape a degraded Also noted, not changed here: a review pass observed that Gates on |
Mechanics repair, not an expectation change. The behaviour under test is unchanged and still asserted: a valid, within-TTL override short-circuits verification, routes through consolidate as RE_ASSERTED, and lands in the by-reviewer stats. No assertion was weakened, widened, skipped, or deleted. Three tests hardcoded asserted_at "2026-05-29T22:00:00+00:00" while _override_is_expired sweeps the overrides namespace against the real wall clock with a 90-day TTL. The fixture was therefore a time bomb: it went red on 2026-08-27 with no code change, and the suite has been red on main ever since (main last ran CI 2026-08-17, green). A test that fails on a calendar date rather than a behaviour change was never pinning what it claimed to. Both sides of the comparison are now literals -- the fixture timestamp and the clock -- so the gap stays a constant 3 days forever. The TTL gate itself is untouched, and the tests whose subject IS the TTL keep offsetting from the real clock on purpose. Testing: uv run pytest plugins/ -- 2116 passed, 0 failed (first green run since 2026-08-27). Mutation: forcing _override_is_expired to return True turns all three repaired tests red, so the pin did not neuter the gate they depend on. ruff check; uv run pyright 0 errors. Risks: none identified. Test-only change.
|
The behaviour under test is unchanged and still asserted. A valid, within-TTL override still short-circuits verification ( What was actually wrong. Three tests hardcoded The repair. Both sides of the comparison are now literals — the fixture timestamp and the clock ( Proof the pin did not neuter the gate. Forcing
|
1 participant
Summary
/code-review's signal-extraction cache served one review's signals to a different review. Thesignals/namespace key was(diff_tip, taxonomy_hash, prompt_hash)anddiff_tipis a ref name, never a commit id —cmd_resolve_scopeleaves it as the literal"HEAD"for every local branch review, and sets it toorigin/<head_ref>for a PR. So no component of the key varied with the diff. The cache directory is~/.claude/cr-cache-global-repo-<basename ofgit rev-parse --show-toplevel>, i.e. the worktree directory name. Any two reviews run from the same directory therefore shared both the directory and the key, and the second was handed the first'sextract_signals.json— describing a diff that was never under review. Pooled worktrees make that the normal case, not an edge case.signal_input_hash: a SHA-256 over the canonical serialization of the agent input bundleextract-signals-preparebuilds (files, per-file added/removed excerpts, intent hint). That bundle is exactly what the extraction agent reads, so the key covers every input the extraction is a function of, and a different diff produces a different key even at an identical path, branch, and tip.extract-signals-preparenow builds the bundle before computing the key rather than only on a miss.cache_keypluscache_bypass_reason: "no-changed-files"in the manifest, which suppresses the read and — throughextract-signals-consolidate's existingif cache_key:guard — the write.--diff-tiphelp, which claimed the flag was a SHA.Breaking changes
None for callers —
--diff-tipis still accepted and still a key component. Cached extractions written under the old key scheme become misses, so the first review in each cache directory re-runs one Haiku extraction.signal_extraction_cache_key()takes a fourth argument; it has no callers outside this module and its tests.UI Feature Flag
[ui-flag:N/A]— tooling-only change in a Python helper; no UI surface in this repo.Test plan
TestPooledWorktreeSignalCacheIsolation::test_second_lane_does_not_receive_first_lanes_signalsreproduces production shape: one cache directory,diff_tip="HEAD"for both lanes, two different diffs. Lane 1 prepares + consolidates real signals into the cache; lane 2 must get a different key,status: "needs_agent", and noextract_signals.json.code_review_helpers.pyreverted (git stash push -- <that file>), that test fails and lane 2's manifest reads"status": "cache_hit"— the production bug, reproduced. Restored, green.test_unkeyable_input_bypasses_the_cache_in_both_directionsdrives the empty-diff path through prepare and consolidate and asserts nothing lands in thesignals/namespace for a later lane to hit.test_key_composition_is_pinned_to_a_literal_digest,test_hash_is_pinned_to_a_literal_digest) rather than re-derived through the helper under test, so dropping the input-hash component back out of the tuple cannot stay green.test_cache_hit_serves_directly_without_agentno longer re-derives the key to seed its fixture; it prepares once and seeds under the key the manifest reports.golden_prefix_empty_diff/expected/extract_signals_manifest.jsonupdated: that fixture's prefix run has no changed files, so it now recordscache_bypass_reason. Deliberate behavior change in this diff.ruff check .clean;uv run pyright0 errors.uv run pytest plugins/: 3 failures, all pre-existing on cleanmainand unrelated —TestOverrideCache::test_verify_prepare_short_circuits_on_valid_overrideand twoTestPR114ReviewFixescases pin an override fixture dated2026-05-29T22:00:00+00:00against a 90-dayoverridesTTL, so they became a wall-clock time bomb around 2026-08-27.